Skip to content

Reset contributed in BreakerState::reset_line - #854

Merged
nicoburns merged 2 commits into
linebender:mainfrom
DioxusLabs:devin/1790619679-reset-line-buffers
Sep 29, 2026
Merged

nicoburns merged 2 commits into
linebender:mainfrom
DioxusLabs:devin/1790619679-reset-line-buffers

Conversation

@nicoburns

Copy link
Copy Markdown
Collaborator

LLM Contributions: Generated with Opus 5.5 High. Reviewed by me.

This is a small refactor motivated by code quality. Previously an &mut reference to contributed was being passed into LineBoxMetrics's reset method. This is a violation of separation of concerns. Resetting contributed now happens in the reset_line method of BreakerState which actually owns contributed.

Changelog: None (pure refactor)

@nicoburns
nicoburns requested review from DJMcNab and tomcur and a lite review from Copilot and removed request for Copilot September 28, 2026 20:35

@DJMcNab DJMcNab left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reset applying the strut also doesn't read great here.

That is, I think reset_line should be the caller of add_strut (although I'm also not entirely convinced by add_strut; it seems like the strut should arise naturally from adding a box from a style - especially if lines with only inline boxes don't want the struts anyway...)

@staging-devin-ai-integration
staging-devin-ai-integration Bot force-pushed the devin/1790619679-reset-line-buffers branch from e614d8f to 1c869cb Compare September 29, 2026 12:58
@nicoburns

Copy link
Copy Markdown
Collaborator Author

especially if lines with only inline boxes don't want the struts anyway...

FWIW, I believe that lines with only inline boxes do want the strut

@nicoburns
nicoburns enabled auto-merge September 29, 2026 13:02
@nicoburns
nicoburns added this pull request to the merge queue Sep 29, 2026
Merged via the queue into linebender:main with commit 30b3055 Sep 29, 2026
24 checks passed
@DJMcNab

DJMcNab commented Sep 29, 2026

Copy link
Copy Markdown
Member

FWIW, I believe that lines with only inline boxes do want the strut

Hmm, I thought this was the logic about lines with absolutely no glyphs. I guess I'm still confused about that change!
But sure, that isn't too important for us.

nicoburns added a commit to DioxusLabs/parley that referenced this pull request Oct 2, 2026
LLM Contributions: Generated with Fable 5.1 Low

Depends on:
- linebender#854

## Context

This is a follow-up to linebender#766
(`vertical-align`) that gets us back to performance parity with before
that PR was merged. It was purposefully left as a follow-up to make
things easier to review, but we probably need to land this or some
alternative performance fix before we can release.

## Summary

Eliminate `SmallVec<[SubtreeExtents; 2]>` on `LineBoxMetrics`, allowing
it to impl `Copy` which is performance-critical because it is copied at
every line breaking opportunity.

## New design

- `LineBoxMetrics` loses the general ``SmallVec<[SubtreeExtents; 2]>``
and gains `root: SubtreeExtents`. So it still stores metrics for the
root subtree on the line but extra subtrees generated by
`vertical-align: top | bottom` move out.
- `LineBoxMetrics` also gains `non_root_height: f32`, the current height
of the tallest of those extra subtrees. This saves `line_height()` from
having to compute it (which would otherwise be much more expensive in
this new design). It is copied into saved break opportunities restored
when reverting to one.
- `BreakerState` gains `subtrees: SubtreeHistory` for the extra
subtrees. This is per-line state that is processed in `finish_line` and
then reset for the next line. All of the bookkeeping for it is inside
`SubtreeHistory`:
- It contains a `Vec<SubtreeExtents>` in which the current extents of a
subtree are the *last* entry with its root.
- `grow()` adds a box to a subtree. If the subtree's most recent entry
in the `Vec` is frozen (see below) it pushes the extents as a new entry,
otherwise it overwrites the entry in place. So entries only pile up when
there are line-breaking opportunities between them.
- `save()` is called at each line-breaking opportunity. It returns the
current length, and marks every entry pushed so far as frozen (so they
can safely be restored when taking a line-breaking opportunity)
- `restore(save)` is called when taking a line-breaking opportunity. It
truncates to the saved length, which drops everything pushed since. The
entries before the save point are guaranteed to be unmodified as they
were frozen when the save point was saved.
- `current()` collects the latest entry for each root into a `SmallVec`
once per-line in `finish_line`.
- `PrevBoundaryState` (saved state for each line-break opportunity)
gains `subtrees_save: usize`, the value returned by `save()` at that
opportunity.
 
## Performance

This is -25% on our existing line-breaking benchmarks.
 
The below are stress tests with hundreds of `vertical-align: top` spans
on a single line, each containing inline boxes of increasing height:
 
| Case | This PR |
| --- | ---: |
| nowrap, 2048 spans, no boxes | 0.99× |
| nowrap, 2048 spans × 8 boxes | 0.34× |
| wrap, 2048 spans, no boxes | 0.51× |
| wrap, 2048 spans × 8 boxes | 0.45× |
| nowrap, 512 spans, 8192 boxes in the first | 1.18× |
| wrap, 512 spans, 8192 boxes in the first | 5.85× |

As you can see, there is a pathological case. That *may* be fixable, but
at the cost of quite a big of complexity and slightly slower performance
in the common cases. I've judged it not worth it for now.

**Changelog**: None (performance improvement for never-published
regression)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants